Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for fission on android #14765

Closed

Conversation

keith
Copy link
Member

@keith keith commented Feb 9, 2022

This change adds the per_object_debug_info feature to the android cc
toolchain to support fission.

This also removes this same feature from the macOS crosstool because
clang targeting darwin does nothing when passed this flag, which leads
to failures if you enable this for android, but are on a macOS host
because host tools fail to produce the dwo files.

@keith keith requested a review from ahumesky as a code owner February 9, 2022 02:10
keith added a commit to envoyproxy/envoy-mobile that referenced this pull request Feb 9, 2022
When building the jni dylib for android, we previously stripped all
debug info to decrease the artifact size. With this change we now
produce the same stripped binary as before, but before stripping it we
create a dump of the debug info suitable for crash reporting.

This is made overly difficult for a few reasons:

1. Bazel doesn't support fission for Android bazelbuild/bazel#14765
2. Extra outputs from rules are not propagated up the dependency tree,
   so just building `android_dist` at the top level, isn't enough to get
   the extra outputs built as well
3. Building the library manually alongside the android artifact on the
   command line results in 2 separate builds, one for android as a
   transitive dependency of `android_dist` and one for the host
   platform

This change avoids #1 fission for now, but the same approach could be used
once that change makes its way to a bazel release.

This change ignores #2 but fixes #3 so it requires you to explicitly
build the library as part of the command line invocation if you want
debug info, like:

```
./bazelw build --fat_apk_cpu=... android_dist //library/common/jni:libenvoy_jni.so.debug_info
```

Theoretically we could probably shove this artifact into the aar to make
sure it was correctly produced, but that risks us accidentally shipping
that in the aar.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
keith added a commit to envoyproxy/envoy-mobile that referenced this pull request Feb 9, 2022
When building the jni dylib for android, we previously stripped all
debug info to decrease the artifact size. With this change we now
produce the same stripped binary as before, but before stripping it we
create a dump of the debug info suitable for crash reporting.

This is made overly difficult for a few reasons:

1. Bazel doesn't support fission for Android bazelbuild/bazel#14765
2. Extra outputs from rules are not propagated up the dependency tree,
   so just building `android_dist` at the top level, isn't enough to get
   the extra outputs built as well
3. Building the library manually alongside the android artifact on the
   command line results in 2 separate builds, one for android as a
   transitive dependency of `android_dist` and one for the host
   platform

This change avoids #1 fission for now, but the same approach could be used
once that change makes its way to a bazel release.

This change fixes #2 by using a separate output group that can be depended
on by the genrule that writes to dist while avoiding #3 because the custom
rule producing these uses the android transition.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith keith force-pushed the ks/add-support-for-fission-on-android branch from 114a18c to 258c60e Compare February 9, 2022 22:56
This change adds the per_object_debug_info feature to the android cc
toolchain to support fission.

This also removes this same feature from the macOS crosstool because
clang targeting darwin does nothing when passed this flag, which leads
to failures if you enable this for android, but are on a macOS host
because host tools fail to produce the dwo files.
@keith keith force-pushed the ks/add-support-for-fission-on-android branch from 258c60e to e5e3fdd Compare February 9, 2022 22:56
@Bencodes
Copy link
Contributor

cc: @ahumesky

@sgowroji sgowroji added the team-Android Issues for Android team label Mar 22, 2022
@sgowroji sgowroji added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 21, 2022
@keith keith requested a review from ted-xie as a code owner April 22, 2022 11:45
@keith
Copy link
Member Author

keith commented Sep 13, 2022

This one is also supported by the new NDK rules https://github.com/bazelbuild/rules_android_ndk/blob/40e17b3af75406ab10219313f16a404937f09874/ndk_cc_toolchain_config.bzl#L886-L894 so I will close this once it's more clear those are the future

jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
When building the jni dylib for android, we previously stripped all
debug info to decrease the artifact size. With this change we now
produce the same stripped binary as before, but before stripping it we
create a dump of the debug info suitable for crash reporting.

This is made overly difficult for a few reasons:

1. Bazel doesn't support fission for Android bazelbuild/bazel#14765
2. Extra outputs from rules are not propagated up the dependency tree,
   so just building `android_dist` at the top level, isn't enough to get
   the extra outputs built as well
3. Building the library manually alongside the android artifact on the
   command line results in 2 separate builds, one for android as a
   transitive dependency of `android_dist` and one for the host
   platform

This change avoids #1 fission for now, but the same approach could be used
once that change makes its way to a bazel release.

This change fixes #2 by using a separate output group that can be depended
on by the genrule that writes to dist while avoiding #3 because the custom
rule producing these uses the android transition.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
@keith
Copy link
Member Author

keith commented Jan 10, 2023

dropping for the new ndk rules

@keith keith closed this Jan 10, 2023
@keith keith deleted the ks/add-support-for-fission-on-android branch January 10, 2023 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Android Issues for Android team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants